Allow edition parsing #4
Conversation
defaulting to proto2 behavior
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces support for Protocol Buffers editions in the Wire library, focusing on maintaining backward compatibility while allowing for future editions. Key changes include:
- Modified
Syntaxclass from enum to sealed class inwire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt - Added support for parsing 'edition' declarations in
wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt - Updated
ProtoFileElementto handle 'edition' keyword inwire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt - Modified
SyntaxRulesto accommodate newSyntax.Editioncase inwire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt - Updated various Java files to use
Syntax.PROTO_2.INSTANCEinstead ofSyntax.PROTO_2
30 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings
| public static final Double DEFAULT_LENGTH_METERS = 0.0d; | ||
|
|
||
| public static final Double DEFAULT_MASS_KILOGRAMS = 0.0d; |
There was a problem hiding this comment.
style: Consider using BigDecimal for precise representation of floating-point numbers
| case 1: return CRETACEOUS; | ||
| case 2: return JURASSIC; | ||
| case 3: return TRIASSIC; | ||
| default: return null; |
There was a problem hiding this comment.
style: Returning null for unknown values might lead to NullPointerExceptions. Consider throwing an IllegalArgumentException instead.
| @@ -0,0 +1,17 @@ | |||
| syntax = "proto2"; | |||
|
|
|||
| package squareup.java.dinosaurs; | |||
There was a problem hiding this comment.
style: Package name differs from other similar files. Consider using 'squareup.dinosaurs' for consistency.
|
|
||
| package squareup.java.dinosaurs; | ||
|
|
||
| import "squareup/wire/java/geology/period.proto"; |
There was a problem hiding this comment.
style: Import path differs from other files. Consider using 'squareup/geology/period.proto' for consistency.
|
|
||
| optional double length_meters = 3; | ||
| optional double mass_kilograms = 4; | ||
| optional squareup.java.geology.Period period = 5; |
There was a problem hiding this comment.
logic: Period type should be 'squareup.geology.Period' to match the typical package structure.
| @Deprecated("Use get(string: String, edition: Boolean) instead", ReplaceWith("Syntax.get(string, edition)")) | ||
| operator fun get(string: String): Syntax { | ||
| for (syntax in values()) { | ||
| if (syntax.string == string) return syntax | ||
| } | ||
| if (string == PROTO_2.toString()) return PROTO_2 | ||
| if (string == PROTO_3.toString()) return PROTO_3 | ||
| throw IllegalArgumentException("unexpected syntax: $string") | ||
| } |
There was a problem hiding this comment.
logic: The deprecated get() function doesn't handle Edition cases, which might cause issues for existing code
| fun get(string: String, edition: Boolean): Syntax { | ||
| if (edition) { | ||
| if (string in KNOWN_EDITIONS) return Edition(string) | ||
| throw IllegalArgumentException("unknown edition: $string") | ||
| } else { | ||
| if (string == PROTO_2.toString()) return PROTO_2 | ||
| if (string == PROTO_3.toString()) return PROTO_3 | ||
| throw IllegalArgumentException("unexpected syntax: $string") | ||
| } | ||
| } |
There was a problem hiding this comment.
style: Consider using a when expression for better readability and maintainability
| when (syntax) { | ||
| is Edition -> append("edition") | ||
| else -> append("syntax") | ||
| } |
There was a problem hiding this comment.
logic: This when expression doesn't handle all possible cases of Syntax. Consider adding an else branch to handle unexpected types
| (label == "syntax" || label == "edition") && context.permitsSyntax() -> { | ||
| reader.expect(syntax == null, location) { "too many syntax or edition definitions" } |
There was a problem hiding this comment.
style: Consider separating the handling of 'syntax' and 'edition' into separate conditions for clarity
| label == "syntax" && context.permitsSyntax() -> { | ||
| reader.expect(syntax == null, location) { "too many syntax definitions" } | ||
| (label == "syntax" || label == "edition") && context.permitsSyntax() -> { | ||
| reader.expect(syntax == null, location) { "too many syntax or edition definitions" } |
There was a problem hiding this comment.
style: Update error message to mention both 'syntax' and 'edition'
defaulting to proto2 behavior, which is kinda wrong.
The way we break the world for Java callers is very sad...
Maybe we could just manually increate the enum values one bye one each year...